Skip to content

Conversation

jpraet
Copy link
Member

@jpraet jpraet commented Dec 6, 2020

Fixes #8861.

Use ajax on PR review page for submitting review comments, adding replies and (un)resolving conversations, instead of refreshing the entire page on each interaction.

@codecov-io
Copy link

codecov-io commented Dec 6, 2020

Codecov Report

Merging #13877 (a8becbe) into master (3c96a37) will decrease coverage by 0.03%.
The diff coverage is 26.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13877      +/-   ##
==========================================
- Coverage   41.85%   41.81%   -0.04%     
==========================================
  Files         743      743              
  Lines       79393    79453      +60     
==========================================
- Hits        33228    33223       -5     
- Misses      40703    40766      +63     
- Partials     5462     5464       +2     
Impacted Files Coverage Δ
modules/auth/repo_form.go 39.83% <ø> (ø)
routers/repo/pull_review.go 0.00% <0.00%> (ø)
models/issue_comment.go 52.28% <62.06%> (-0.44%) ⬇️
routers/routes/macaron.go 93.18% <100.00%> (+<0.01%) ⬆️
modules/util/timer.go 42.85% <0.00%> (-42.86%) ⬇️
modules/queue/manager.go 62.13% <0.00%> (-2.96%) ⬇️
modules/charset/charset.go 68.53% <0.00%> (-2.25%) ⬇️
services/gitdiff/gitdiff.go 68.99% <0.00%> (-1.94%) ⬇️
routers/api/v1/repo/pull.go 24.84% <0.00%> (-0.61%) ⬇️
services/pull/pull.go 42.57% <0.00%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c96a37...a8becbe. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 6, 2020
@lafriks lafriks added the topic/ui Change the appearance of the Gitea UI label Dec 7, 2020
@lafriks lafriks added this to the 1.14.0 milestone Dec 7, 2020
@silverwind
Copy link
Member

silverwind commented Dec 9, 2020

Some suggestions. Mostly async ajax requests and simplification.
diff --git a/web_src/js/index.js b/web_src/js/index.js
index fbbd97bdf..b94bf527c 100644
--- a/web_src/js/index.js
+++ b/web_src/js/index.js
@@ -1275,9 +1275,9 @@ function initPullRequestReview() {
     e.preventDefault();
     $(this).closest('.menu').toggle('visible');
   });

-  $('a.add-code-comment').on('click', function (e) {
+  $('a.add-code-comment').on('click', async function (e) {
     if ($(e.target).hasClass('btn-add-single')) return; // https://github.com/go-gitea/gitea/issues/4745
     e.preventDefault();

     const isSplit = $(this).closest('.code-diff').hasClass('code-diff-split');
@@ -1308,22 +1308,20 @@ function initPullRequestReview() {

     const td = ntr.find(`.add-comment-${side}`);
     let commentCloud = td.find('.comment-code-cloud');
     if (commentCloud.length === 0 && !ntr.find('button[name="is_review"]').length) {
-      const url = $(this).data('new-comment-url');
-      $.get(url, (data) => {
-        td.html(data);
-        commentCloud = td.find('.comment-code-cloud');
-        assingMenuAttributes(commentCloud.find('.menu'));
-        td.find("input[name='line']").val(idx);
-        td.find("input[name='side']").val(side === 'left' ? 'previous' : 'proposed');
-        td.find("input[name='path']").val(path);
-        const $textarea = commentCloud.find('textarea');
-        attachTribute($textarea.get(), {mentions: true, emoji: true});
-        const $simplemde = setCommentSimpleMDE($textarea);
-        $textarea.focus();
-        $simplemde.codemirror.focus();
-      });
+      const data = await $.get($(this).data('new-comment-url'));
+      td.html(data);
+      commentCloud = td.find('.comment-code-cloud');
+      assingMenuAttributes(commentCloud.find('.menu'));
+      td.find("input[name='line']").val(idx);
+      td.find("input[name='side']").val(side === 'left' ? 'previous' : 'proposed');
+      td.find("input[name='path']").val(path);
+      const $textarea = commentCloud.find('textarea');
+      attachTribute($textarea.get(), {mentions: true, emoji: true});
+      const $simplemde = setCommentSimpleMDE($textarea);
+      $textarea.focus();
+      $simplemde.codemirror.focus();
     }
   });
 }

@@ -2483,31 +2481,26 @@ $(document).ready(async () => {
     e.checked = false;
     $(e).trigger('click');
   });

-  $(document).on('click', '.resolve-conversation', function (e) {
+  $(document).on('click', '.resolve-conversation', async function (e) {
     e.preventDefault();
-    const id = $(this).data('comment-id');
+    const comment_id = $(this).data('comment-id');
     const origin = $(this).data('origin');
     const action = $(this).data('action');
     const url = $(this).data('update-url');

-    $.post(url, {
-      _csrf: csrf,
-      origin,
-      action,
-      comment_id: id,
-    }, (data) => {
-      if ($(this).closest('.conversation-holder').length) {
-        const conversation = $(data);
-        $(this).closest('.conversation-holder').replaceWith(conversation);
-        conversation.find('.dropdown').dropdown();
-        initReactionSelector(conversation);
-        initClipboard();
-      } else {
-        reload();
-      }
-    });
+    const data = await $.post(url, {_csrf: csrf, origin, action, comment_id});

+    if ($(this).closest('.conversation-holder').length) {
+      const conversation = $(data);
+      $(this).closest('.conversation-holder').replaceWith(conversation);
+      conversation.find('.dropdown').dropdown();
+      initReactionSelector(conversation);
+      initClipboard();
+    } else {
+      reload();
+    }
   });

   buttonsClickOnEnter();
   searchUsers();
@@ -3617,27 +3610,23 @@ function initIssueList() {
 $(document).on('click', 'button[name="is_review"]', (e) => {
   $(e.target).closest('form').append('<input type="hidden" name="is_review" value="true">');
 });

-$(document).on('submit', '.conversation-holder form', (e) => {
+$(document).on('submit', '.conversation-holder form', async (e) => {
   e.preventDefault();
   const form = $(e.target);
-  $.post(form.attr('action'), form.serialize(), (data) => {
-    const conversation = $(data);
-    const path = conversation.data('path');
-    const side = conversation.data('side');
-    const idx = conversation.data('idx');
-    const lineType = form.closest('tr').data('line-type');
-    form.closest('.conversation-holder').replaceWith(conversation);
-    if (lineType === 'same') {
-      $(`a.add-code-comment[data-path="${path}"][data-idx="${idx}"]`).addClass('invisible');
-    } else {
-      $(`a.add-code-comment[data-path="${path}"][data-side="${side}"][data-idx="${idx}"]`).addClass('invisible');
-    }
-    conversation.find('.dropdown').dropdown();
-    initReactionSelector(conversation);
-    initClipboard();
-  });
+  const newConversationHolder = $(await $.post(form.attr('action'), form.serialize()));
+  const {path, side, idx} = newConversationHolder.data();

+  form.closest('.conversation-holder').replaceWith(newConversationHolder);
+  if (form.closest('tr').data('line-type') === 'same') {
+    $(`a.add-code-comment[data-path="${path}"][data-idx="${idx}"]`).addClass('invisible');
+  } else {
+    $(`a.add-code-comment[data-path="${path}"][data-side="${side}"][data-idx="${idx}"]`).addClass('invisible');
+  }
+  newConversationHolder.find('.dropdown').dropdown();
+  initReactionSelector(newConversationHolder);
+  initClipboard();
 });

 window.cancelCodeComment = function (btn) {
   const form = $(btn).closest('form');

@jpraet jpraet marked this pull request as draft December 10, 2020 18:38
FetchCodeCommentsByLine was initially more or less copied from fetchCodeCommentsByReview. Now they both use a common findCodeComments function instead
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 10, 2020
@jpraet
Copy link
Member Author

jpraet commented Dec 10, 2020

Rebased because there were some merge conflicts. I also extracted some duplicate code. FetchCodeCommentsByLine was initially more or less copied from fetchCodeCommentsByReview. Now they both use a common findCodeComments function instead.

@jpraet jpraet marked this pull request as ready for review December 10, 2020 21:16
@jpraet
Copy link
Member Author

jpraet commented Jan 6, 2021

ping 👀

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 7, 2021
@lafriks
Copy link
Member

lafriks commented Jan 8, 2021

🚀

@lafriks lafriks changed the title #8861 use ajax on PR review page Do not reload page after adding comments in Pull Request reviews Jan 8, 2021
@lafriks lafriks merged commit bcb7f35 into go-gitea:master Jan 8, 2021
@lafriks lafriks added the type/enhancement An improvement of existing functionality label Jan 8, 2021
a1012112796 added a commit to a1012112796/gitea that referenced this pull request Jan 14, 2021
* master: (252 commits)
  Issues overview should not show issues from archived repos (go-gitea#13220)
  Display SVG files as images instead of text (go-gitea#14101)
  [skip ci] Updated translations via Crowdin
  Update docs to clarify issues raised in go-gitea#14272 (go-gitea#14318)
  [skip ci] Updated translations via Crowdin
  [Refactor] Passwort Hash/Set (go-gitea#14282)
  Add option to change username to the admin panel (go-gitea#14229)
  fix mailIssueCommentBatch for pull request (go-gitea#14252)
  Remove self from MAINTAINERS (go-gitea#14286)
  Do not reload page after adding comments in Pull Request reviews (go-gitea#13877)
  Fix session bug when introduce chi (go-gitea#14287)
  [skip ci] Updated translations via Crowdin
  Add secure/httpOnly attributes to the lang cookie (go-gitea#9690) (go-gitea#14279)
  Some code improvements (go-gitea#14266)
  [skip ci] Updated translations via Crowdin
  Fix wrong type on hooktask to convert typ from char(16) to varchar(16) (go-gitea#14148)
  Upgrade XORM links in documentation. (go-gitea#14265)
  Check permission for the appropriate unit type (go-gitea#14261)
  Add compliance check for windows to ensure cross platform build (go-gitea#14260)
  [skip ci] Updated translations via Crowdin
  ...
@jpraet jpraet deleted the 8861-review-ajax branch January 22, 2021 18:38
@go-gitea go-gitea locked and limited conversation to collaborators Feb 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/ui Change the appearance of the Gitea UI type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review page should not get refresh on adding comment
9 participants